Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix checking ammID field of account #5188

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix checking ammID field of account #5188

wants to merge 2 commits into from
+21 −0

Conversation

oleks-rip
Copy link
Collaborator

High Level Overview of Change

This fix add check for sfAMMID field in amm_info handler.

Context of Change

Without this check debug version of rippled crash on assert in ledger::read()
Bug was found during automation testing.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

API Impact

  • Public API: amm_info will not crash debug version of the rippled

@codecov Codecov
Copy link

codecov bot commented Nov 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.9%. Comparing base (9d58f11) to head (eab4e58).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5188     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          782     782             
  Lines        66616   66616             
  Branches      8161    8158      -3     
=========================================
- Hits         51902   51870     -32     
- Misses       14714   14746     +32
Files with missing lines Coverage Δ
src/xrpld/rpc/handlers/AMMInfo.cpp 92.6% <100.0%> (+0.1%) ⬆️

... and 9 files with indirect coverage changes

Impacted file tree graph

gregtatcam
gregtatcam approved these changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@kennyzlei kennyzlei added this to the 2.3.0 (Nov 2024) milestone Nov 13, 2024
@shawnxie999
Copy link
Collaborator

Unrelated to this change, but on line 162, an non existent AMM object is returned with rpcACT_NOT_FOUND error. Doesn't sound right, should we fix it?

        auto const amm = ledger->read(ammKeylet);
        if (!amm)
            return Unexpected(rpcACT_NOT_FOUND);

shawnxie999
shawnxie999 reviewed Nov 14, 2024
View reviewed changes
src/test/rpc/AMMInfo_test.cpp
Comment on lines +322 to +334
using namespace jtx;
testcase("Invalid amm field");

testAMM([&](AMM& amm, Env&) {
auto const resp = amm.ammRpcInfo(
std::nullopt,
jss::validated.c_str(),
std::nullopt,
std::nullopt,
gw);
BEAST_EXPECT(
resp.isMember("error") && resp["error"] == "actNotFound");
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the test could be moved into testErrors() IMO

@gregtatcam
Copy link
Collaborator

Unrelated to this change, but on line 162, an non existent AMM object is returned with rpcACT_NOT_FOUND error. Doesn't sound right, should we fix it?

        auto const amm = ledger->read(ammKeylet);
        if (!amm)
            return Unexpected(rpcACT_NOT_FOUND);

It's half correct since AMM might be fetched by amm account. But correct, rpcOBJECT_NOT_FOUND is probably more appropriate.

@shawnxie999
Copy link
Collaborator

Unrelated to this change, but on line 162, an non existent AMM object is returned with rpcACT_NOT_FOUND error. Doesn't sound right, should we fix it?

        auto const amm = ledger->read(ammKeylet);
        if (!amm)
            return Unexpected(rpcACT_NOT_FOUND);

It's half correct since AMM might be fetched by amm account. But correct, rpcOBJECT_NOT_FOUND is probably more appropriate.

although i believe we may need a new API version for this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shawnxie999 shawnxie999 shawnxie999 left review comments

@gregtatcam gregtatcam gregtatcam approved these changes

@godexsoft godexsoft godexsoft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.3.0 (Nov 2024)
Development

Successfully merging this pull request may close these issues.
5 participants
@oleks-rip @shawnxie999 @gregtatcam @godexsoft @kennyzlei